-
Notifications
You must be signed in to change notification settings - Fork 920
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP]fraud/testing: Use header/mocks.Store instead of custom mockStore #1674
[WIP]fraud/testing: Use header/mocks.Store instead of custom mockStore #1674
Conversation
This change modifies the test file in the fraud pkg to make use of mockStore in libs/header/mocks. Fixes: celestiaorg#1542 Signed-off-by: Richard Gregory <richardgrecoson@gmail.com>
It was meant to be a straightforward modification, but a new change made it a little complicated, at least for me. There are now two different header packages:
|
@distractedm1nd do you understand the issue I explained in the comments? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tackling this,@richardgreg, and sorry that it took us so much time to get here.
Also lint is broken + needs rebase
store := &mockStore{ | ||
headers: make(map[int64]*header.ExtendedHeader), | ||
headHeight: 0, | ||
Headers: make(map[int64]H), | ||
HeadHeight: 0, | ||
} | ||
|
||
suite := headertest.NewTestSuite(t, numHeaders) | ||
|
||
for i := 0; i < numHeaders; i++ { | ||
header := suite.GenExtendedHeader() | ||
store.headers[header.Height()] = header | ||
store.Headers[header.Height()] = header | ||
|
||
if header.Height() > store.headHeight { | ||
store.headHeight = header.Height() | ||
if header.Height() > store.HeadHeight { | ||
store.HeadHeight = header.Height() | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use mocks.NewStore
instead. It already implements header gen, when testsuite is passed to it as Generator.
} | ||
type H libheader.Header | ||
|
||
type mockStore mockstore.MockStore[H] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to define another type can use MockStore directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we wouldn't be able to define methods on them; at least, that's the error I get when I call headerMock.MockStore
instead of the custom mockStore
p.s. My knowledge of GO isn't deep, so if I sound too naive, I can skip this problem and take on another one 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @richardgreg no worries! If you want to define additional methods on MockStore
, you are welcome to add them in libs/header/mocks/store.go
if they are necessary/useful :) So no need to alias the type here. We'd like to keep MockStore
and all its methods within the scope of the libs/header/mocks
package anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @richardgreg are you still working on this? If not, we can take it over no problem!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @renaynay, I kept running into dead ends when attempting to fix this. It's evident I need to improve my GO skills. Yeah, you can take it over. 🙂
@richardgreg, gentle ping |
Hi @Wondertan, still here. I got your feedback; I'll update you tomorrow :) |
@richardgreg, gentle nudge |
Hi @Wondertan, I added a response here 3 days ago. Can I close this so someone else can work on it? |
Closing as it's now fixed by #2039 |
This change modifies the test file in the fraud pkg to make use of mockStore in libs/header/mocks.
Fixes: #1542
Signed-off-by: Richard Gregory richardgrecoson@gmail.com
Overview
Checklist